[fix](be) Restore mutable ownership in COW paths#63001
Draft
zclllyybb wants to merge 4 commits intoapache:masterfrom
Draft
[fix](be) Restore mutable ownership in COW paths#63001zclllyybb wants to merge 4 commits intoapache:masterfrom
zclllyybb wants to merge 4 commits intoapache:masterfrom
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I found a blocking COW correctness issue in complex-type deserialization. The PR changes these paths from mutating the existing child through assume_mutable() to mutating a possibly cloned child, but the cloned child is not written back to the parent complex column. That means deserialization can update offsets while dropping child data when nested children are shared, which is exactly the ownership case this PR is trying to make safe.
Critical checkpoint conclusions:
- Goal/test: The goal is to restore COW-safe mutation after assume_mutable() enforces exclusive ownership. The broad BE unit-test list helps, but the complex-type deserialize path still has an uncovered shared-child case.
- Scope/focus: The change is focused on COW ownership, but several deserialize updates need the same write-back pattern used elsewhere after mutating detached children.
- Concurrency/lifecycle/config/compatibility: No new concurrency, lifecycle, configuration, or storage-format compatibility concern identified in the reviewed changes.
- Parallel paths: Array, Map, and Struct deserialize have the same detached-child pattern and all need to be fixed consistently.
- Tests: Please add or adjust coverage for deserializing complex columns whose child ColumnPtr is shared so this does not regress.
- Observability/persistence/write correctness/performance: No additional observability, persistence, transaction, or performance blocker found beyond the data-correctness issue above.
User focus: No additional user-provided review focus was specified.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
I found several issues that should be addressed before merging. The existing inline threads already cover the Array/Map/Struct COW deserialization ownership problems, so I did not duplicate them.
Critical checkpoint conclusions:
- Goal/test: The PR appears to make BE column mutation safe under COW and remove streaming-job filtered-row persistence. It is only partially achieved: the ORC schema-change nullable path can copy stale null flags, and filtered-row observability is left inconsistent. Existing tests were updated broadly, but the streaming filtered-row regression test was deleted without replacement coverage for the new expected behavior.
- Scope/focus: The BE COW change is broad and not fully localized; this increases risk in hot scan/convert paths.
- Concurrency/lifecycle: I did not find new lock-order or thread-lifecycle issues in the reviewed paths.
- Config/incompatibility: No new config items. The protobuf field removal needs reserved tag/name handling to avoid future wire/persistence compatibility hazards.
- Parallel paths: Filtered-row updates were removed from transactional streaming jobs, while non-transactional streaming offset commits and metric definitions still keep filtered rows, leaving behavior inconsistent.
- Tests/results: Missing replacement coverage for the removed streaming filtered-row behavior. I did not run tests in this review runner.
- Observability: Filtered-row metrics/job details can now report zero or omit data even when rows are filtered.
User focus: No additional user-provided review focus was specified.
| auto converted_column = doris_column->assume_mutable(); | ||
| if (converted_column->is_nullable()) { | ||
| align_orc_null_map(resolved_column, | ||
| reinterpret_cast<ColumnNullable*>(converted_column.get()), |
Contributor
There was a problem hiding this comment.
This copies the source null map from offset 0, but in the non-consistent nullable converter path the source nullable column was built with the destination null map and then deep-mutated. If doris_column already has rows, fill_orc_null_map() appends the newly-read ORC nulls after those old rows in resolved_column; passing only resolved_column->size() here then makes align_orc_null_map() insert the first new_rows entries, which are stale old null flags rather than the newly appended slice. This can corrupt null flags when reading multiple batches through an ORC schema-change conversion. Please preserve the destination old row count and copy from that offset, or avoid cloning a source nullable wrapper that contains the destination's existing null map.
Issue Number: N/A
Related PR: N/A
Problem Summary: assume_mutable now asserts exclusive ownership, so paths that moved columns out of blocks or borrowed shared subcolumns must return ownership explicitly or mutate through owning COW handles. This restores COW-safe mutation in block reuse, variant extraction, parquet/orc conversion, memtable aggregation, compaction readers, result buffering, and affected BE unit tests.
None
- Test: Unit Test
- ./run-be-ut.sh --run -j 100
- ./run-be-ut.sh --run --filter=LocalExchangerTest.*
- ./run-be-ut.sh --run --filter=ArrowResultBlockBufferTest.*:BitUtil.CountZero:Parameters/TestRowIdConversion.*
- ./run-be-ut.sh --run --filter=VariantColumnWriterReaderTest.*:HierarchicalDataIteratorTest.*
- ./run-be-ut.sh --run --filter=VariantColumnWriterReaderTest.test_nested_iter_nullable:VariantColumnWriterReaderTest.test_streaming_compaction_writer_streams_regular_array_paths_across_batches:TabletCooldownTest.normal
- PATH=/mnt/disk6/common/ldb_toolchain_toucan/bin:/mnt/disk3/zhaochangle/.opencode/bin:/mnt/disk3/zhaochangle/.local/bin:/mnt/disk6/common/apache-maven-3.9.14/bin:/mnt/disk6/common/ldb_toolchain_028/bin:/mnt/disk6/common/jdk-17.0.16/bin:/mnt/disk6/common/node-v24.14.1-linux-x64/bin:/mnt/disk3/zhaochangle/.bun/bin:/mnt/disk3/zhaochangle/.opencode/bin:/mnt/disk3/zhaochangle/.local/bin:/mnt/disk6/common/apache-maven-3.9.14/bin:/mnt/disk6/common/ldb_toolchain_028/bin:/mnt/disk6/common/jdk-17.0.16/bin:/mnt/disk6/common/node-v24.14.1-linux-x64/bin:/mnt/disk6/common/apache-maven-3.9.14/bin:/mnt/disk6/common/ldb_toolchain_028/bin:/mnt/disk6/common/jdk-17.0.16/bin:/mnt/disk6/common/node-v24.14.1-linux-x64/bin:/mnt/disk3/zhaochangle/.codex/tmp/arg0/codex-arg0w0dN9c:/mnt/disk6/common/node-v24.14.1-linux-x64/lib/node_modules/@openai/codex/node_modules/@openai/codex-linux-x64/vendor/x86_64-unknown-linux-musl/path:/mnt/disk3/zhaochangle/.bun/bin:/mnt/disk3/zhaochangle/.opencode/bin:/mnt/disk3/zhaochangle/.local/bin:/mnt/disk6/common/apache-maven-3.9.14/bin:/mnt/disk6/common/ldb_toolchain_028/bin:/mnt/disk6/common/jdk-17.0.16/bin:/mnt/disk6/common/node-v24.14.1-linux-x64/bin:/mnt/disk6/common/apache-maven-3.9.14/bin:/mnt/disk6/common/ldb_toolchain_028/bin:/mnt/disk6/common/jdk-17.0.16/bin:/mnt/disk6/common/node-v24.14.1-linux-x64/bin:/mnt/disk3/zhaochangle/.opencode/bin:/mnt/disk3/zhaochangle/.local/bin:/mnt/disk3/zhaochangle/.local/bin:/mnt/disk3/zhaochangle/bin:/mnt/disk6/common/apache-maven-3.9.14/bin:/mnt/disk6/common/ldb_toolchain_028/bin:/mnt/disk6/common/jdk-17.0.16/bin:/mnt/disk6/common/node-v24.14.1-linux-x64/bin:/usr/share/Modules/bin:/usr/lib64/ccache:/usr/local/bin:/usr/bin:/usr/local/sbin:/usr/sbin ./build-support/clang-format.sh
- git diff --cached --check
- PATH=/mnt/disk6/common/ldb_toolchain_toucan/bin:/mnt/disk3/zhaochangle/.opencode/bin:/mnt/disk3/zhaochangle/.local/bin:/mnt/disk6/common/apache-maven-3.9.14/bin:/mnt/disk6/common/ldb_toolchain_028/bin:/mnt/disk6/common/jdk-17.0.16/bin:/mnt/disk6/common/node-v24.14.1-linux-x64/bin:/mnt/disk3/zhaochangle/.bun/bin:/mnt/disk3/zhaochangle/.opencode/bin:/mnt/disk3/zhaochangle/.local/bin:/mnt/disk6/common/apache-maven-3.9.14/bin:/mnt/disk6/common/ldb_toolchain_028/bin:/mnt/disk6/common/jdk-17.0.16/bin:/mnt/disk6/common/node-v24.14.1-linux-x64/bin:/mnt/disk6/common/apache-maven-3.9.14/bin:/mnt/disk6/common/ldb_toolchain_028/bin:/mnt/disk6/common/jdk-17.0.16/bin:/mnt/disk6/common/node-v24.14.1-linux-x64/bin:/mnt/disk3/zhaochangle/.codex/tmp/arg0/codex-arg0w0dN9c:/mnt/disk6/common/node-v24.14.1-linux-x64/lib/node_modules/@openai/codex/node_modules/@openai/codex-linux-x64/vendor/x86_64-unknown-linux-musl/path:/mnt/disk3/zhaochangle/.bun/bin:/mnt/disk3/zhaochangle/.opencode/bin:/mnt/disk3/zhaochangle/.local/bin:/mnt/disk6/common/apache-maven-3.9.14/bin:/mnt/disk6/common/ldb_toolchain_028/bin:/mnt/disk6/common/jdk-17.0.16/bin:/mnt/disk6/common/node-v24.14.1-linux-x64/bin:/mnt/disk6/common/apache-maven-3.9.14/bin:/mnt/disk6/common/ldb_toolchain_028/bin:/mnt/disk6/common/jdk-17.0.16/bin:/mnt/disk6/common/node-v24.14.1-linux-x64/bin:/mnt/disk3/zhaochangle/.opencode/bin:/mnt/disk3/zhaochangle/.local/bin:/mnt/disk3/zhaochangle/.local/bin:/mnt/disk3/zhaochangle/bin:/mnt/disk6/common/apache-maven-3.9.14/bin:/mnt/disk6/common/ldb_toolchain_028/bin:/mnt/disk6/common/jdk-17.0.16/bin:/mnt/disk6/common/node-v24.14.1-linux-x64/bin:/usr/share/Modules/bin:/usr/lib64/ccache:/usr/local/bin:/usr/bin:/usr/local/sbin:/usr/sbin ./build-support/check-format.sh (attempted; blocked by pre-existing formatting drift in be/src/exec/operator/distinct_streaming_aggregation_operator.cpp)
- PATH=/mnt/disk6/common/ldb_toolchain_toucan/bin:/mnt/disk3/zhaochangle/.opencode/bin:/mnt/disk3/zhaochangle/.local/bin:/mnt/disk6/common/apache-maven-3.9.14/bin:/mnt/disk6/common/ldb_toolchain_028/bin:/mnt/disk6/common/jdk-17.0.16/bin:/mnt/disk6/common/node-v24.14.1-linux-x64/bin:/mnt/disk3/zhaochangle/.bun/bin:/mnt/disk3/zhaochangle/.opencode/bin:/mnt/disk3/zhaochangle/.local/bin:/mnt/disk6/common/apache-maven-3.9.14/bin:/mnt/disk6/common/ldb_toolchain_028/bin:/mnt/disk6/common/jdk-17.0.16/bin:/mnt/disk6/common/node-v24.14.1-linux-x64/bin:/mnt/disk6/common/apache-maven-3.9.14/bin:/mnt/disk6/common/ldb_toolchain_028/bin:/mnt/disk6/common/jdk-17.0.16/bin:/mnt/disk6/common/node-v24.14.1-linux-x64/bin:/mnt/disk3/zhaochangle/.codex/tmp/arg0/codex-arg0w0dN9c:/mnt/disk6/common/node-v24.14.1-linux-x64/lib/node_modules/@openai/codex/node_modules/@openai/codex-linux-x64/vendor/x86_64-unknown-linux-musl/path:/mnt/disk3/zhaochangle/.bun/bin:/mnt/disk3/zhaochangle/.opencode/bin:/mnt/disk3/zhaochangle/.local/bin:/mnt/disk6/common/apache-maven-3.9.14/bin:/mnt/disk6/common/ldb_toolchain_028/bin:/mnt/disk6/common/jdk-17.0.16/bin:/mnt/disk6/common/node-v24.14.1-linux-x64/bin:/mnt/disk6/common/apache-maven-3.9.14/bin:/mnt/disk6/common/ldb_toolchain_028/bin:/mnt/disk6/common/jdk-17.0.16/bin:/mnt/disk6/common/node-v24.14.1-linux-x64/bin:/mnt/disk3/zhaochangle/.opencode/bin:/mnt/disk3/zhaochangle/.local/bin:/mnt/disk3/zhaochangle/.local/bin:/mnt/disk3/zhaochangle/bin:/mnt/disk6/common/apache-maven-3.9.14/bin:/mnt/disk6/common/ldb_toolchain_028/bin:/mnt/disk6/common/jdk-17.0.16/bin:/mnt/disk6/common/node-v24.14.1-linux-x64/bin:/usr/share/Modules/bin:/usr/lib64/ccache:/usr/local/bin:/usr/bin:/usr/local/sbin:/usr/sbin ./build-support/run-clang-tidy.sh --build-dir be/ut_build_ASAN (attempted; blocked by existing clang-tidy analyzer errors from be/src/util/jni-util.h static_assert(false) and pre-existing diagnostics)
- Behavior changed: No
- Does this need documentation: No
Issue Number: None Related PR: apache#63001 Problem Summary: After the COW mutable ownership changes, constructing a MutableBlock from the cached Iceberg equality-delete block transfers the block columns into the mutable block. The equality-delete loader merged rows into the mutable block but did not write the result back, leaving null columns in the cached block and crashing HybridSet while building delete filters. This also applies the requested BE clang-format updates. None - Test: Manual test - build-support/check-format.sh - ./build.sh --be -j 100 - git diff --check - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
zclllyybb
added a commit
to zclllyybb/doris
that referenced
this pull request
May 8, 2026
…sion Issue Number: None Related PR: apache#63001 Problem Summary: After assume_mutable started asserting exclusive ownership, sort aggregate state still appended through immutable Block columns on the hot add/merge path, and nullable schema-change conversion could copy a stale null-map prefix after ORC/Parquet COW cloning. Keep the sort aggregate state as a MutableBlock, remove a read-only mutable assertion, and copy nullable null-map slices from the appended source range. Release note None Check List (For Author) - Test: Unit Test - ./run-be-ut.sh --run --filter=AggregateFunctionSortDataTest.merge_does_not_share_rhs_block:OrcReaderFillDataTest.SchemaChangeNullableNullMapUsesAppendedSlice:ParquetColumnConvertTest.AlignNullMapUsesAppendedSourceSlice -j 100 - ./run-regression-test.sh --run -f regression-test/suites/query_p0/runtimefilterV2/rfv2.groovy - ./build.sh --be -j 100 - build-support/clang-format.sh - build-support/check-format.sh - git diff --check - Local cluster restart and select 1/show backends - build-support/run-clang-tidy.sh --build-dir be/build_ASAN attempted; changed-line findings were fixed, remaining failures are existing ORC/test include/jni-util baseline or tooling diagnostics - Behavior changed: No - Does this need documentation: No
…sion Issue Number: None Related PR: apache#63001 Problem Summary: After assume_mutable started asserting exclusive ownership, sort aggregate state still appended through immutable Block columns on the hot add/merge path, and nullable schema-change conversion could copy a stale null-map prefix after ORC/Parquet COW cloning. Keep the sort aggregate state as a MutableBlock, remove a read-only mutable assertion, and copy nullable null-map slices from the appended source range. Release note None Check List (For Author) - Test: Unit Test - ./run-be-ut.sh --run --filter=AggregateFunctionSortDataTest.merge_does_not_share_rhs_block:OrcReaderFillDataTest.SchemaChangeNullableNullMapUsesAppendedSlice:ParquetColumnConvertTest.AlignNullMapUsesAppendedSourceSlice -j 100 - ./run-regression-test.sh --run -f regression-test/suites/query_p0/runtimefilterV2/rfv2.groovy - ./build.sh --be -j 100 - build-support/clang-format.sh - build-support/check-format.sh - git diff --check - Local cluster restart and select 1/show backends - build-support/run-clang-tidy.sh --build-dir be/build_ASAN attempted; changed-line findings were fixed, remaining failures are existing ORC/test include/jni-util baseline or tooling diagnostics - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary: assume_mutable now asserts exclusive ownership, so paths that moved columns out of blocks or borrowed shared subcolumns must return ownership explicitly or mutate through owning COW handles. This restores COW-safe mutation in block reuse, variant extraction, parquet/orc conversion, memtable aggregation, compaction readers, result buffering, and affected BE unit tests.